Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add OCI image with Loom on Python3 #6

Merged
merged 5 commits into from
May 2, 2024

Conversation

ships
Copy link
Contributor

@ships ships commented Apr 19, 2024

This PR adds a Nix derivation for a layered OCI image that contains Loom and its dependencies for python3.

Note that currently, Loom's dependency distributions does not have a compilation pathway for ARM architectures, so although this defines packages on all architectures, you must build the image with .#packages.x86_64-linux.ociImgLoom.

Note that about half of the changes here are a refactor to support use of callPy3Package from crossPkgsLinux.

@ships ships force-pushed the ships/chore/add-oci-img-loom branch from 8a63b1e to 2e88a03 Compare April 19, 2024 20:28
Copy link
Contributor

@srounce srounce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly fine, left some suggestions to make things a bit more idiomatic.

flake.nix Show resolved Hide resolved
Comment on lines +32 to +35
goftests = callPackage ./../goftests { };
parsable = callPackage ./../parsable { };
pymetis = callPackage ./../pymetis { };
distributions = callPackage ./../distributions {inherit goftests parsable;};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're gonna use a custom scoped callPackage it seems like it'd be a bit more idiomatic to have a attrset of private packages merged into the callPackage scope. That way packages within the flake can use the args injection to reference them from other packages without having to deal with the paths, but they won't be exposed directly under the flake packages field.

flake.nix Outdated Show resolved Hide resolved
pkgs/oci/inferenceql.loom/default.nix Outdated Show resolved Hide resolved
pkgs/oci/inferenceql.loom/default.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
pkgs/oci/inferenceql.loom/default.nix Outdated Show resolved Hide resolved
in pkgs.dockerTools.buildImage {
name = "probcomp/inferenceql.loom";
tag = systemWithLinux;
fromImage = ociImgBase;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not using fromImage and just building a single layered image for each instead by composing in whatever base config in Nix-land. You'll end up with a smaller image that's easier to reason about.

parsable
;
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
passthru.ociImage = callPackage ./ociImage.nix { };

So you'd end up with nix build .#loom.ociImage rather than a separate sibling package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I like the inheritance here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am weighing this abstraction against like .#oci.loom ...

@ships ships force-pushed the ships/chore/add-oci-img-loom branch from 49fbeea to d6d5b4d Compare April 25, 2024 23:16
@ships ships requested a review from Schaechtle April 26, 2024 00:58
@ships
Copy link
Contributor Author

ships commented Apr 26, 2024

@Schaechtle , would love your take on these. I think i am inclined to pull it in as-is and address some of the modularization @srounce talks about later, because it will be better decided once we have more context about how central this repo is compared to nix-code in each component repo.

@Schaechtle
Copy link

Schaechtle commented Apr 29, 2024

@Schaechtle , would love your take on these. I think i am inclined to pull it in as-is and address some of the modularization @srounce talks about later, because it will be better decided once we have more context about how central this repo is compared to nix-code in each component repo.

yes. That sounds right to me.

@Schaechtle
Copy link

FYI, the following runs fine on my machine

nix build .#ociImgLoom

@ships ships force-pushed the ships/chore/add-oci-img-loom branch from d6d5b4d to 5ee8fd9 Compare May 1, 2024 16:00
@ships ships force-pushed the ships/chore/add-oci-img-loom branch from 2abd7fa to 9e19987 Compare May 2, 2024 19:12
@ships ships merged commit d99bfec into main May 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants